refactor(cni): extract galactic-bgp as its own CNI chain plugin by privateip · Pull Request #305 · datum-cloud/galactic

privateip · 2026-08-08T04:22:51Z

Stack (merge bottom to top):

refactor(cni): split galactic-cni into veth/tap master plugins #303 refactor(cni): split galactic-cni into veth/tap master plugins
refactor(cni): extract galactic-ipam as a real delegated CNI IPAM plugin #304 refactor(cni): extract galactic-ipam as a real delegated CNI IPAM plugin
refactor(cni): extract galactic-bgp as its own CNI chain plugin #305 refactor(cni): extract galactic-bgp as its own CNI chain plugin (this PR)
refactor(cni): extract galactic-route as its own CNI chain plugin #306 refactor(cni): extract galactic-route as its own CNI chain plugin
docs+fixes(cni): finish the CNI plugin-chain split — docs, containerlab, dead code #307 docs+fixes(cni): finish the CNI plugin-chain split - docs, containerlab, dead code

Summary

Third branch in the CNI plugin-chain split stack (based on #304). galactic-bgp is now its own chained CNI plugin, invoked last in the conflist after the master plugin and galactic-ipam, instead of in-process BGP/SRv6/eBPF publishing inside galactic-cni/galactic-tap-cni.

What moved

internal/cnibgp mirrors cniipam's shape: RunPlugin(), PluginConf{VPC, VPCAttachment, Namespace} parsed from stdin. It reuses internal/config.CNIConfig's GALACTIC_CNI_* env vars — shared node-level settings, not a BGP-specific flag.

galactic-bgp never touches the container netns. It learns interface kind (veth vs tap) and the allocated IPAM result from prevResult alone (prevresult.go): two interfaces means veth, one means tap. It publishes BGP state, then passes prevResult through unchanged as the last plugin in the chain.

cmdCheck is new: verifies the BGPVRFInstance/BGPAdvertisement CRDs and the eBPF vrf_table entry. cmdStatus probes the API server, matching STATUS across the chain. cmdDel stays a no-op — orphaned VRF/CRD state is reconciled by galactic-router's GC controller, not by any CNI DEL path.

Rollback scoping

resourceTracker in cnibgp/resource.go covers only what galactic-bgp's own ADD creates: BGPVRFInstance, BGPAdvertisement, the eBPF vrf_table entry. internal/cni/resource.go's tracker drops to vpc, vpcAttachment, vrfCreated, routesCreated and no longer needs a Kubernetes client.

hostgw extraction

ConfigureHostGateway configures the container's default route — kernel-interface work galactic-bgp shouldn't depend on. It now lives in internal/cni/hostgw, called directly by both master plugins before handing off down the chain.

PrevResult vs RawPrevResult

types.PluginConf.PrevResult has JSON tag "-" and is never populated; only RawPrevResult map[string]interface{} is. Pre-existing library quirk, not introduced here — ops_check.go already reads RawPrevResult, and inferFromPrevResult follows the same pattern. Out of scope to fix.

Verification

task lint ✅ task build ✅ task test:unit ✅
task test:e2e not run, same caveat as refactor(cni): split galactic-cni into veth/tap master plugins #303/refactor(cni): extract galactic-ipam as a real delegated CNI IPAM plugin #304.

🤖 Generated with Claude Code

mattdjenkinson · 2026-08-08T16:09:53Z

Reviewed this one as well. A few issues here, some of which seem worth blocking on.

The rollback context in internal/cnibgp/ops_add.go:47 is created once at the top of cmdAdd with a 10s timeout, but publishBGPState's retryK8sOps can take up to about 30s (three attempts, each with its own fresh 10s context, plus backoff). If the k8s API is slow enough that retries run out before a hard failure surfaces, the deferred tracker.cleanup(rollbackCtx) ends up calling Delete with a context that's already expired. That fails with "context deadline exceeded" rather than NotFound, so client.IgnoreNotFound doesn't catch it, and the BGPVRFInstance/BGPAdvertisement CRDs that were just created leak instead of rolling back. Separate but related: rollbackCancel() only gets called in the error branch, so every successful ADD leaks that context's timer for up to 10 seconds.

CHECK doesn't verify everything ADD writes. The new checkEBPFEntry in internal/cnibgp/ops_check.go:96 only reads back registry.VRF.Get, never the locator or function tables, and skips the nodeID range validation that ADD treats as a hard error. If the locator/function tables get corrupted or go missing while the VRF table survives, or if nodeID drifts out of range after the fact, CHECK still reports the attachment healthy. That's a false positive on the SRv6 datapath.

internal/cnibgp/prevresult.go:44 introduces a new hard version constraint that didn't exist before. inferFromPrevResult goes through type100.NewResult, which only accepts a cniVersion of exactly 1.0.0 or 1.1.0. Previously BGP publish got the IPAM result as a plain in-process Go value with no version dependency at all. Now any conflist using something else, 0.4.0 for instance, which the master plugins would happily print, makes galactic-bgp's ADD fail for every attachment in the chain.

DEL doesn't respect the conflist's actual version either. internal/cnibgp/ops_del.go:26 never parses args.StdinData and always prints 1.0.0, unlike internal/cni/ops_del.go, which only falls back to that on a parse failure and otherwise uses pluginConf.CNIVersion. Since pluginConf is never parsed here, DEL also never logs vpc/vpcAttachment, which makes failures harder to trace back to a specific attachment.

There's also an e2e coverage gap with a comment that no longer matches reality. tests/e2e/e2e_test.go:214 says eBPF registration happens "inline from galactic-tap-cni's own cmdAdd," but this PR moved that into the separately chain-invoked galactic-bgp binary, and the test still only execs /galactic-tap-cni directly, never /galactic-bgp. The test keeps passing because it only checks the printed CNI result's interfaces and IPs, but it's quietly lost all coverage of BGP CRD creation and eBPF vrf_table registration on the ADD path.

Interface-kind inference is fragile too. internal/cnibgp/prevresult.go:53 decides veth versus tap purely from the interface count (one means tap, two means veth), and the assumption that galactic-bgp always runs immediately after the master plugin is only written down in comments, never enforced in code. #306 is about to add galactic-route into this same chain. If it, or anything added later, changes the interface count galactic-bgp sees, veth/tap gets silently misclassified and the wrong eBPF redirect kind gets programmed instead of the call failing loudly.

Smaller things: internal/cni/resource.go:29 (and identically internal/cnitap/resource.go:26) still registers bgpv1alpha1.AddToScheme even though this PR removes every BGP CRD read/write from that package, and the comment justifying it is circular. No test in the new internal/cnibgp package exercises resourceTracker.cleanup's actual wiring, only the standalone unregisterEBPFDatapath function is tested directly, and the old test that covered this end to end (including a rollback collision race) wasn't replaced. publishResult and resourceTracker declare the same five fields and cmdAdd copies them one by one instead of embedding one in the other, so a future field added to one but not the other stops being tracked with no compiler error to catch it. And egressKindForInterfaceType's empty-string branch is dead code left over from when interface type was optional; its only caller now never produces an empty string.

The rollback-context bug and the lost e2e coverage for BGP and eBPF registration seem worth blocking on. The version-constraint and DEL-version issues are at least worth a comment, even if they turn out to be non-issues given how conflists actually get authored in practice.

Rollback context (blocking): rollbackCtx was created up front with a single 10s budget and reused for tracker.cleanup() after publishBGPState's own retryK8sOps could already burn ~30s across its retries. A slow API server could leave cleanup with an expired context, turning Delete's "NotFound" into "context deadline exceeded" (which client.IgnoreNotFound doesn't catch), leaking the just-created CRDs. rollbackCancel was also only called on the error branch, leaking a timer on every successful ADD. Now rollbackCtx is created fresh, with its own full budget, only inside the failure branch. CHECK coverage (blocking): checkEBPFEntry only read back vrf_table, so a corrupted/missing locator_table or function_table entry, or a nodeID that drifted out of range after ADD, still reported the attachment healthy. It now also verifies locator_table, function_table, and the nodeID range, matching what ADD treats as a hard error. e2e coverage (blocking): TestCNITapInterface's comment claimed eBPF registration happens inline in galactic-tap-cni's cmdAdd, which stopped being true with the plugin-chain split, and the test never exercised galactic-bgp at all -- silently losing coverage of BGP CRD creation and eBPF registration on the ADD path. It now chains /galactic-bgp ADD (fed the tap master's real result as prevResult) and CHECK after the tap master's own ADD, asserting the BGPVRFInstance/BGPAdvertisement CRDs exist and that CHECK -- which reads back all three eBPF tables -- passes. cniVersion constraint: type100.NewResult only accepts "1.0.0"/"1.1.0", and the master plugin echoes the conflist's own cniVersion straight into its printed Result, so an older value fails galactic-bgp's ADD for every attachment. Documented as an explicit, intentional requirement in prevresult.go and docs/cni/configuration.md, with a test locking in the rejection of older versions. DEL version/logging: cmdDel never parsed args.StdinData, always printed cniVersion "1.0.0" unconditionally (unlike internal/cni/ops_del.go, which only falls back to that on a parse failure), and never logged vpc/vpcAttachment. It now parses the conflist, logs the attachment, and uses pluginConf.CNIVersion with the same fallback-on-parse-failure pattern as the other DEL implementations in this chain. Interface-kind inference: veth/tap was inferred purely from len(Interfaces) (1 vs 2), enforced only in comments -- #306 chaining galactic-route into this same prevResult could add a host-side interface and silently misclassify tap as veth (or vice versa) instead of failing loudly, since both counts are valid switch cases. inferFromPrevResult now counts Sandbox-carrying interfaces instead: the actual property that distinguishes veth (guest end moved into the container netns) from tap (host-only), which survives an extra host-side interface without misclassifying. Smaller cleanup: - Removed the stale bgpv1alpha1.AddToScheme registration (with a circular justifying comment) from internal/cni/resource.go and internal/cnitap/resource.go -- neither package touches BGP CRDs anymore. - Added resource_test.go covering resourceTracker.cleanup's own wiring end to end (all three resource kinds, plus the rollback-collision race at that level) -- previously only the standalone unregisterEBPFDatapath was tested directly. - resourceTracker now embeds publishResult instead of cmdAdd copying its five tracking fields over one by one, so a future field added to one can't silently stop being tracked in the other. - Removed egressKindForInterfaceType's dead empty-string branch -- inferFromPrevResult always produces "veth" or "tap" now. Also added prevresult_test.go and ops_del_test.go, and introduced a package-level ebpfPinDir var (defaulting to attach.PinDir) so tests can redirect galactic-bgp's own eBPF registration/rollback/CHECK reads to a throwaway pin directory instead of the real production one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

privateip · 2026-08-09T20:11:06Z

@mattdjenkinson comments address, ready for review

CNI_NETNS_OVERRIDE (blocking): galactic-route never entered any netns, so cmd/galactic-route/main.go assumed it never needed the stdin peek-and-repipe dance or CNI_NETNS_OVERRIDE that galactic-cni/ galactic-tap-cni use. That's true for veth-mode attachments, where CNI_NETNS points at the container's netns and differs from this process's own ambient (host) netns. It's false for tap-mode attachments: CNI_NETNS is deliberately set to the host's own root netns there (no per-VM netns exists), which equals this process's ambient netns, so skel's post-Add/Del same-netns check rejected every tap-mode ADD/DEL with terminations even though the route was already installed correctly. Now peeks stdin for interface_type the same way galactic-cni does and sets CNI_NETNS_OVERRIDE=true only for tap mode. CHECK on-link routes: checkTerminationRoutes unconditionally called net.ParseIP on Via and errored on nil, but Via is omitempty and assembleRoute (route.go) has a real branch that installs a valid on-link route for an empty Via, which cmdAdd installs fine. CHECK always failed with "invalid termination gateway" for those regardless. Restructured the match loop to treat an empty Via as looking for a gateway-less, device-scoped route instead of erroring immediately. Carried over byte for byte from the pre-split internal/cni/ops_check.go (also reachable via internal/cnitap), so this fixes the same bug there too by virtue of the code having moved. Docs: docs/cni/configuration.md still listed terminations as a galactic-cni/galactic-tap-cni field and showed it inline in the master's own JSON, which this PR's PluginConf split made wrong -- the master's slimmer struct silently drops the field on unmarshal, so an operator following the doc gets a silent no-op. Moved the field out of the master's Top-Level Fields table, reworded Termination Fields to attribute it to galactic-route's own conflist stanza (including that cmdDel is a no-op, not "deleted in reverse order"), and rewrote the worked example as a chained plugins array. Deferred per the review: the internal/cniroute/config.go:205 dead `if conf.PrevResult != nil` branch is copy-pasted across internal/cni, internal/cnitap, and internal/cnibgp too, predating this PR -- left for #307, which already scopes "dead code" cleanup for the chain split. Verification: task lint, task build (all 8 binaries), task test:unit all pass. task test:e2e not run, same caveat as #303/#304/#305 -- this repo has no root/CAP_NET_ADMIN available, and the existing checkTerminationRoutes tests already can't get past the vrf.TableID lookup without a real kernel VRF, so the on-link CHECK fix has no new automated regression test beyond what task test:e2e's Kind cluster would exercise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ab, dead code Rebased onto the updated refactor/cni-chain-3-galactic-route (which picked up PR #306's own review-feedback fix commit, fe72a1e) after that branch's history moved out from under this one. Conflicts resolved: - All 11 containerlab tenant NAD manifests: kept this PR's plugins-array chain wrapper (adding the galactic-bgp stage) around the nested "ipam": {"type": "galactic-ipam", ...} block that PR #305's own fix commit had already introduced independently of this PR -- the two changes were orthogonal, so the merge is additive. - docs/cni/configuration.md: took this PR's fuller rewrite throughout (it supersedes PR #306's narrower doc fix -- e.g. this PR already covers the interface_type/terminations field removal and the renamed GALACTIC_IPAM_ENABLE_LOCAL_IPAM env var more completely), but preserved two things #306 fixed that this PR's diff predates and doesn't otherwise cover: the cniVersion 1.0.0/1.1.0 prevResult constraint paragraph, and "on-link route" (not "link-local route" -- fd01::/48 in the example isn't a link-local address) in the terminations example. - tests/e2e/e2e_test.go: this PR's diff removed startEBPFControlDaemon (call, definition, and the attach import) on the theory that TestCNITapInterface never touches the eBPF datapath. That was true when this PR's diff was authored, but PR #305's own fix commit (7cf773a) had independently added testChainedGalacticBGP, chaining galactic-bgp after the tap master's ADD and asserting BGPVRFInstance/ BGPAdvertisement CRD creation -- and registerEBPFDatapath's usidmap.OpenPinnedRegistry only opens already-pinned maps, it never loads/pins the eBPF program itself, so testChainedGalacticBGP can't succeed without startEBPFControlDaemon having run first. Restored the call, its definition, and the import, and updated the doc comments this PR had already rewritten (which claimed the test "does not chain into ... galactic-bgp") to describe the merged reality instead. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Rebased onto the updated refactor/cni-chain-4-installer-docs (which picked up PR #307's own review-feedback fix commit) after that branch's history moved out from under this one -- same ripple as #306 -> #307. Conflicts resolved: internal/cni/resource.go and internal/cnitap/ resource.go both had two independent changes touching the same struct/cleanup() region -- this PR's own removal of vrfCreated/VRF deletion (since the VRF is now shared per-VPC, not per-attachment, so a single attachment's rollback must never delete it), and PR #305/#306's unrelated addition of ipamDelegated/ipamType/ipamStdin for real IPAM-delegation rollback. Kept both: dropped vrfCreated and the VRF-delete step, kept the IPAM rollback fields and step, and kept this PR's fuller "why no VRF deletion" doc comment (it explains the shared-VRF reasoning more completely than the version already in these files). internal/cnibgp/bgp.go had one similar conflict at the registerEBPFDatapath call site -- resolved to drop the vpcAttachment argument (this PR's change) while keeping the ebpfPinDir package var (added by #305's own fix, for test injection) rather than reverting to the attach.PinDir literal this PR's diff predates. Also fixed one file this PR's diff never touched: internal/cnibgp/ resource_test.go didn't exist yet at this PR's original base -- it was added later by #305's own fix commit -- so its direct vrf.Add/ vrf.Delete/vrf.TableID(vpc, vpcAttachment) calls needed the same vpcAttachment-arg removal this PR already applied everywhere else, or the package wouldn't build. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nadpatch Rebased onto the updated refactor/cni-chain-4-installer-docs (which picked up PR #307's own review-feedback fix commit) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #311. Conflicts resolved: internal/cnibgp/bgp.go, ops_check.go, and resource.go each had an import-block conflict from this PR's internal/cni/crdnames -> internal/crdnames promotion landing on lines the current tree had already changed independently (PR #305's own fix commit moved cnibgp's eBPF pin-dir handling behind a package- level ebpfPinDir var in cnibgp.go, so bgp.go/ops_check.go/resource.go no longer import internal/plumbing/ebpf/attach directly the way this PR's diff -- authored before that -- expected). Resolved by applying just the crdnames rename to each file and leaving the attach import out, matching how the current tree already gets ebpfPinDir. Also fixed one file this PR's diff never touched: internal/cnibgp/ resource_test.go didn't exist yet at this PR's original base -- it was added later by #305's own fix commit -- so its own "go.datum.net/galactic/internal/cni/crdnames" import needed the same promotion this PR already applied everywhere else, or the package wouldn't build. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the three promoted packages (internal/crdnames, internal/hostconf, internal/nadpatch). tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Rebased onto the updated fix/vrf-shared-per-vpc-1-core (which picked up my own rebase-and-reconcile of that branch after PR #307 moved out from under it) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #311 -> #312. Conflicts resolved: this PR drops the eBPF vrf_table rollback path (registerEBPFDatapath no longer returns a block to track, unregisterEBPFDatapath is deleted entirely, publishResult loses ebpfRegistered/ebpfBlock/ebpfArgument) since the vrf_table entry is now shared per (VPC, node) same as the BGPVRFInstance CRD, so a failed ADD must never unregister it. The current tree had independently refactored resourceTracker to embed publishResult (rather than copying its fields one-by-one) between this PR's original base and now, so I kept that embedding -- it still holds exactly this PR's two surviving fields (advertisementCreated, vrfInstanceCreated) once the eBPF fields are gone -- and added this PR's own vrfInstanceCreated-conditioned-on-OperationResultCreated behavior and nodeName field on top of it, along with this PR's fuller cleanup() doc comment (it explains the shared-VRF reasoning more completely than what was already there). Also kept ebpfPinDir (the package-level var #305's own fix added for test injection) over the attach.PinDir literal this PR's diff predates, matching the same resolution #311 needed one level up. internal/cnibgp/resource_test.go needed a full rewrite rather than a per-hunk merge: it didn't exist yet at this PR's original base either (same gap #311 hit) -- it was added by #305's own fix commit -- so this PR's diff shows the file as "new," and the version already in the tree still tested the old unconditional-vrfInstanceCreated/ ebpfRegistered design this PR removes. Took this PR's four tests wholesale (they're purpose-built for the new design) and adjusted their resourceTracker literals for the embedded-publishResult shape (publishResult: publishResult{vrfInstanceCreated: true} instead of a bare vrfInstanceCreated: true field, which the embedding makes illegal in a keyed literal). Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nimaster Rebased onto the updated fix/cni-review-followups-doc-placement (which picked up my own rebase-and-reconcile of that branch after PR #307 moved out from under it) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #315 -> #316. Conflicts resolved: internal/cni/resource.go and internal/cnitap/ resource.go each had two independent changes touching the same resourceTracker/cleanup() region -- this PR's own extraction of the shared k8s-client-construction (newK8sClient/cniScheme) and interface+ VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an IPAM-release rollback step (this PR's diff predates that feature entirely, same gap #311/#312/#315 each hit one level up). Kept both: call cnimaster.CleanupAttachment for the interface+VRF half (this PR's whole point), and kept the IPAM release step ahead of it, unchanged. internal/cnitap/ops_check.go had one similar import-only conflict (this PR drops the netlink/rest/ctrl/vrf imports cnimaster. CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) -- its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this- PR gap) sat entirely outside the conflicted hunk and needed no resolution beyond keeping the "github.com/containernetworking/plugins/ pkg/ipam" import alive. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the new internal/cnimaster package. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

CNI_NETNS_OVERRIDE (blocking): galactic-route never entered any netns, so cmd/galactic-route/main.go assumed it never needed the stdin peek-and-repipe dance or CNI_NETNS_OVERRIDE that galactic-cni/ galactic-tap-cni use. That's true for veth-mode attachments, where CNI_NETNS points at the container's netns and differs from this process's own ambient (host) netns. It's false for tap-mode attachments: CNI_NETNS is deliberately set to the host's own root netns there (no per-VM netns exists), which equals this process's ambient netns, so skel's post-Add/Del same-netns check rejected every tap-mode ADD/DEL with terminations even though the route was already installed correctly. Now peeks stdin for interface_type the same way galactic-cni does and sets CNI_NETNS_OVERRIDE=true only for tap mode. CHECK on-link routes: checkTerminationRoutes unconditionally called net.ParseIP on Via and errored on nil, but Via is omitempty and assembleRoute (route.go) has a real branch that installs a valid on-link route for an empty Via, which cmdAdd installs fine. CHECK always failed with "invalid termination gateway" for those regardless. Restructured the match loop to treat an empty Via as looking for a gateway-less, device-scoped route instead of erroring immediately. Carried over byte for byte from the pre-split internal/cni/ops_check.go (also reachable via internal/cnitap), so this fixes the same bug there too by virtue of the code having moved. Docs: docs/cni/configuration.md still listed terminations as a galactic-cni/galactic-tap-cni field and showed it inline in the master's own JSON, which this PR's PluginConf split made wrong -- the master's slimmer struct silently drops the field on unmarshal, so an operator following the doc gets a silent no-op. Moved the field out of the master's Top-Level Fields table, reworded Termination Fields to attribute it to galactic-route's own conflist stanza (including that cmdDel is a no-op, not "deleted in reverse order"), and rewrote the worked example as a chained plugins array. Deferred per the review: the internal/cniroute/config.go:205 dead `if conf.PrevResult != nil` branch is copy-pasted across internal/cni, internal/cnitap, and internal/cnibgp too, predating this PR -- left for #307, which already scopes "dead code" cleanup for the chain split. Verification: task lint, task build (all 8 binaries), task test:unit all pass. task test:e2e not run, same caveat as #303/#304/#305 -- this repo has no root/CAP_NET_ADMIN available, and the existing checkTerminationRoutes tests already can't get past the vrf.TableID lookup without a real kernel VRF, so the on-link CHECK fix has no new automated regression test beyond what task test:e2e's Kind cluster would exercise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ab, dead code Rebased onto the updated refactor/cni-chain-3-galactic-route (which picked up PR #306's own review-feedback fix commit, fe72a1e) after that branch's history moved out from under this one. Conflicts resolved: - All 11 containerlab tenant NAD manifests: kept this PR's plugins-array chain wrapper (adding the galactic-bgp stage) around the nested "ipam": {"type": "galactic-ipam", ...} block that PR #305's own fix commit had already introduced independently of this PR -- the two changes were orthogonal, so the merge is additive. - docs/cni/configuration.md: took this PR's fuller rewrite throughout (it supersedes PR #306's narrower doc fix -- e.g. this PR already covers the interface_type/terminations field removal and the renamed GALACTIC_IPAM_ENABLE_LOCAL_IPAM env var more completely), but preserved two things #306 fixed that this PR's diff predates and doesn't otherwise cover: the cniVersion 1.0.0/1.1.0 prevResult constraint paragraph, and "on-link route" (not "link-local route" -- fd01::/48 in the example isn't a link-local address) in the terminations example. - tests/e2e/e2e_test.go: this PR's diff removed startEBPFControlDaemon (call, definition, and the attach import) on the theory that TestCNITapInterface never touches the eBPF datapath. That was true when this PR's diff was authored, but PR #305's own fix commit (7cf773a) had independently added testChainedGalacticBGP, chaining galactic-bgp after the tap master's ADD and asserting BGPVRFInstance/ BGPAdvertisement CRD creation -- and registerEBPFDatapath's usidmap.OpenPinnedRegistry only opens already-pinned maps, it never loads/pins the eBPF program itself, so testChainedGalacticBGP can't succeed without startEBPFControlDaemon having run first. Restored the call, its definition, and the import, and updated the doc comments this PR had already rewritten (which claimed the test "does not chain into ... galactic-bgp") to describe the merged reality instead. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Rebased onto the updated refactor/cni-chain-4-installer-docs (which picked up PR #307's own review-feedback fix commit) after that branch's history moved out from under this one -- same ripple as #306 -> #307. Conflicts resolved: internal/cni/resource.go and internal/cnitap/ resource.go both had two independent changes touching the same struct/cleanup() region -- this PR's own removal of vrfCreated/VRF deletion (since the VRF is now shared per-VPC, not per-attachment, so a single attachment's rollback must never delete it), and PR #305/#306's unrelated addition of ipamDelegated/ipamType/ipamStdin for real IPAM-delegation rollback. Kept both: dropped vrfCreated and the VRF-delete step, kept the IPAM rollback fields and step, and kept this PR's fuller "why no VRF deletion" doc comment (it explains the shared-VRF reasoning more completely than the version already in these files). internal/cnibgp/bgp.go had one similar conflict at the registerEBPFDatapath call site -- resolved to drop the vpcAttachment argument (this PR's change) while keeping the ebpfPinDir package var (added by #305's own fix, for test injection) rather than reverting to the attach.PinDir literal this PR's diff predates. Also fixed one file this PR's diff never touched: internal/cnibgp/ resource_test.go didn't exist yet at this PR's original base -- it was added later by #305's own fix commit -- so its direct vrf.Add/ vrf.Delete/vrf.TableID(vpc, vpcAttachment) calls needed the same vpcAttachment-arg removal this PR already applied everywhere else, or the package wouldn't build. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Rebased onto the updated fix/vrf-shared-per-vpc-1-core (which picked up my own rebase-and-reconcile of that branch after PR #307 moved out from under it) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #311 -> #312. Conflicts resolved: this PR drops the eBPF vrf_table rollback path (registerEBPFDatapath no longer returns a block to track, unregisterEBPFDatapath is deleted entirely, publishResult loses ebpfRegistered/ebpfBlock/ebpfArgument) since the vrf_table entry is now shared per (VPC, node) same as the BGPVRFInstance CRD, so a failed ADD must never unregister it. The current tree had independently refactored resourceTracker to embed publishResult (rather than copying its fields one-by-one) between this PR's original base and now, so I kept that embedding -- it still holds exactly this PR's two surviving fields (advertisementCreated, vrfInstanceCreated) once the eBPF fields are gone -- and added this PR's own vrfInstanceCreated-conditioned-on-OperationResultCreated behavior and nodeName field on top of it, along with this PR's fuller cleanup() doc comment (it explains the shared-VRF reasoning more completely than what was already there). Also kept ebpfPinDir (the package-level var #305's own fix added for test injection) over the attach.PinDir literal this PR's diff predates, matching the same resolution #311 needed one level up. internal/cnibgp/resource_test.go needed a full rewrite rather than a per-hunk merge: it didn't exist yet at this PR's original base either (same gap #311 hit) -- it was added by #305's own fix commit -- so this PR's diff shows the file as "new," and the version already in the tree still tested the old unconditional-vrfInstanceCreated/ ebpfRegistered design this PR removes. Took this PR's four tests wholesale (they're purpose-built for the new design) and adjusted their resourceTracker literals for the embedded-publishResult shape (publishResult: publishResult{vrfInstanceCreated: true} instead of a bare vrfInstanceCreated: true field, which the embedding makes illegal in a keyed literal). Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nadpatch Rebased onto the updated refactor/cni-chain-4-installer-docs (which picked up PR #307's own review-feedback fix commit) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #311. Conflicts resolved: internal/cnibgp/bgp.go, ops_check.go, and resource.go each had an import-block conflict from this PR's internal/cni/crdnames -> internal/crdnames promotion landing on lines the current tree had already changed independently (PR #305's own fix commit moved cnibgp's eBPF pin-dir handling behind a package- level ebpfPinDir var in cnibgp.go, so bgp.go/ops_check.go/resource.go no longer import internal/plumbing/ebpf/attach directly the way this PR's diff -- authored before that -- expected). Resolved by applying just the crdnames rename to each file and leaving the attach import out, matching how the current tree already gets ebpfPinDir. Also fixed one file this PR's diff never touched: internal/cnibgp/ resource_test.go didn't exist yet at this PR's original base -- it was added later by #305's own fix commit -- so its own "go.datum.net/galactic/internal/cni/crdnames" import needed the same promotion this PR already applied everywhere else, or the package wouldn't build. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the three promoted packages (internal/crdnames, internal/hostconf, internal/nadpatch). tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nimaster Rebased onto the updated fix/cni-review-followups-doc-placement (which picked up my own rebase-and-reconcile of that branch after PR #307 moved out from under it) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #315 -> #316. Conflicts resolved: internal/cni/resource.go and internal/cnitap/ resource.go each had two independent changes touching the same resourceTracker/cleanup() region -- this PR's own extraction of the shared k8s-client-construction (newK8sClient/cniScheme) and interface+ VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an IPAM-release rollback step (this PR's diff predates that feature entirely, same gap #311/#312/#315 each hit one level up). Kept both: call cnimaster.CleanupAttachment for the interface+VRF half (this PR's whole point), and kept the IPAM release step ahead of it, unchanged. internal/cnitap/ops_check.go had one similar import-only conflict (this PR drops the netlink/rest/ctrl/vrf imports cnimaster. CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) -- its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this- PR gap) sat entirely outside the conflicted hunk and needed no resolution beyond keeping the "github.com/containernetworking/plugins/ pkg/ipam" import alive. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the new internal/cnimaster package. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

ecv

Approving. Inferring veth vs tap from sandboxed-interface count rather than len(Interfaces) is the right call, and the cniVersion constraint being documented rather than buried is appreciated.

Fast-follow: #331. galactic-bgp refuses to run un-chained, but nothing catches the reverse — a conflist missing it gives you a pod with an interface, addresses, and no VPC datapath, all green.

Nit, no issue filed: cmdDel runs the full parseConf, which can hit DetectNodeNameFromAPI and list up to 1000 nodes, purely to log vpc/vpcattachment.

Good to see e2e actually chaining it against real CRDs now.

Rebased onto the updated fix/vrf-shared-per-vpc-1-core (which picked up my own rebase-and-reconcile of that branch after PR #307 moved out from under it) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #311 -> #312. Conflicts resolved: this PR drops the eBPF vrf_table rollback path (registerEBPFDatapath no longer returns a block to track, unregisterEBPFDatapath is deleted entirely, publishResult loses ebpfRegistered/ebpfBlock/ebpfArgument) since the vrf_table entry is now shared per (VPC, node) same as the BGPVRFInstance CRD, so a failed ADD must never unregister it. The current tree had independently refactored resourceTracker to embed publishResult (rather than copying its fields one-by-one) between this PR's original base and now, so I kept that embedding -- it still holds exactly this PR's two surviving fields (advertisementCreated, vrfInstanceCreated) once the eBPF fields are gone -- and added this PR's own vrfInstanceCreated-conditioned-on-OperationResultCreated behavior and nodeName field on top of it, along with this PR's fuller cleanup() doc comment (it explains the shared-VRF reasoning more completely than what was already there). Also kept ebpfPinDir (the package-level var #305's own fix added for test injection) over the attach.PinDir literal this PR's diff predates, matching the same resolution #311 needed one level up. internal/cnibgp/resource_test.go needed a full rewrite rather than a per-hunk merge: it didn't exist yet at this PR's original base either (same gap #311 hit) -- it was added by #305's own fix commit -- so this PR's diff shows the file as "new," and the version already in the tree still tested the old unconditional-vrfInstanceCreated/ ebpfRegistered design this PR removes. Took this PR's four tests wholesale (they're purpose-built for the new design) and adjusted their resourceTracker literals for the embedded-publishResult shape (publishResult: publishResult{vrfInstanceCreated: true} instead of a bare vrfInstanceCreated: true field, which the embedding makes illegal in a keyed literal). Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nadpatch Rebased onto the updated refactor/cni-chain-4-installer-docs (which picked up PR #307's own review-feedback fix commit) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #311. Conflicts resolved: internal/cnibgp/bgp.go, ops_check.go, and resource.go each had an import-block conflict from this PR's internal/cni/crdnames -> internal/crdnames promotion landing on lines the current tree had already changed independently (PR #305's own fix commit moved cnibgp's eBPF pin-dir handling behind a package- level ebpfPinDir var in cnibgp.go, so bgp.go/ops_check.go/resource.go no longer import internal/plumbing/ebpf/attach directly the way this PR's diff -- authored before that -- expected). Resolved by applying just the crdnames rename to each file and leaving the attach import out, matching how the current tree already gets ebpfPinDir. Also fixed one file this PR's diff never touched: internal/cnibgp/ resource_test.go didn't exist yet at this PR's original base -- it was added later by #305's own fix commit -- so its own "go.datum.net/galactic/internal/cni/crdnames" import needed the same promotion this PR already applied everywhere else, or the package wouldn't build. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the three promoted packages (internal/crdnames, internal/hostconf, internal/nadpatch). tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nimaster Rebased onto the updated fix/cni-review-followups-doc-placement (which picked up my own rebase-and-reconcile of that branch after PR #307 moved out from under it) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #315 -> #316. Conflicts resolved: internal/cni/resource.go and internal/cnitap/ resource.go each had two independent changes touching the same resourceTracker/cleanup() region -- this PR's own extraction of the shared k8s-client-construction (newK8sClient/cniScheme) and interface+ VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an IPAM-release rollback step (this PR's diff predates that feature entirely, same gap #311/#312/#315 each hit one level up). Kept both: call cnimaster.CleanupAttachment for the interface+VRF half (this PR's whole point), and kept the IPAM release step ahead of it, unchanged. internal/cnitap/ops_check.go had one similar import-only conflict (this PR drops the netlink/rest/ctrl/vrf imports cnimaster. CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) -- its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this- PR gap) sat entirely outside the conflicted hunk and needed no resolution beyond keeping the "github.com/containernetworking/plugins/ pkg/ipam" import alive. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the new internal/cnimaster package. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Step 2 of the CNI plugin-chain split (galactic/plan-cni-plugin-chain): pulls BGP/SRv6/eBPF publishing out of the veth and tap master plugins into its own chained CNI binary, galactic-bgp, invoked last in the conflist after galactic-cni/galactic-tap-cni and galactic-ipam. internal/cnibgp is the new plugin package: - cnibgp.go: RunPlugin() entrypoint (skel.PluginMainFuncs, ADD/DEL/ CHECK/STATUS/VERSION), mirroring the shape of cniipam/cnitap. - types.go, config.go: PluginConf{VPC, VPCAttachment, Namespace}, parsed from stdin. Deliberately reuses internal/config.CNIConfig (GALACTIC_CNI_* env vars) rather than inventing its own prefix like galactic-ipam did — these are shared node-level settings (API server address, namespace, etc.), not a BGP-specific concern, so there is no reason to duplicate or rename them for this binary. - prevresult.go: inferFromPrevResult() reconstructs everything galactic-bgp needs (interface kind — veth vs tap, and the allocated IPAM result) purely from the previous plugin's prevResult, since galactic-bgp itself never touches the container's network namespace. Interface kind is inferred from prevResult shape: two interfaces means veth (host+container), one means tap. - resource.go: a resourceTracker scoped to exactly what galactic-bgp's own ADD creates — BGPVRFInstance, BGPAdvertisement, and the eBPF vrf_table entry. This is intentionally smaller than the old single process-wide tracker: the kernel-interface/VRF cleanup that used to live alongside these now belongs to each master plugin's own tracker (internal/cni, internal/cnitap), scoped to exactly what its own ADD creates. Selective rollback stays correct because each plugin only ever rolls back what it itself created. - ops_add.go/ops_del.go/ops_check.go: cmdAdd parses config, infers from prevResult, publishes BGP state, and passes prevResult through unchanged (galactic-bgp is the last plugin in the chain). cmdDel is a no-op everywhere in the chain, as decided in the plan — orphaned VRF/CRD state is reconciled independently by galactic-router's GC controller, not by any CNI DEL path, so cmdDel does not need to distinguish "resources this plugin created" from anything else. cmdCheck is new logic (the old bgp.go had no CHECK story of its own): it verifies the BGPVRFInstance and BGPAdvertisement CRDs exist and cross-checks the eBPF vrf_table entry via a new checkEBPFEntry helper. cmdStatus probes the API server, matching the STATUS story every other binary in the chain now implements (per plan decision). internal/cnibgp/bgp.go keeps the actual BGP/SRv6/eBPF logic moved over from internal/cni/bgp.go, with everything now unexported since it is package-internal to cnibgp rather than shared across internal/cni: publishConfig, publishResult, publishBGPState, egressKindForInterface- Type, unregisterEBPFDatapath, plus the untouched allocation/collision/ CRD-building helpers (allocateArgument, checkArgumentCollision, lookupBGPRouter, buildVRFInstanceSpec, buildAdvertisementSpec, ipamAdvertisementPrefixes, allAdvertisedPrefixes, registerEBPFDatapath, isTransientError/retryK8sOps). Design refinement beyond the original plan: ConfigureHostGateway and its helpers (installGatewayNeighbor, ipv4GatewayAddrParams, installGatewayRoute, routeConflicts) do NOT move into galactic-bgp. They configure the container's default route to the VRF gateway address, which is kernel-interface/netns work — exactly the kind of dependency the plan's own rationale for splitting BGP out says galactic-bgp should have zero of. Moving them into galactic-bgp would have reintroduced that dependency one step later in the chain instead of removing it. They now live in a new internal/cni/hostgw package, called directly by both master plugins (galactic-cni and galactic-tap-cni) right after they configure the container interface, before handing off down the chain. Also discovered while wiring inferFromPrevResult: types.PluginConf's PrevResult field (from containernetworking/cni/pkg/types) has a json tag of "-" and is never populated by json.Unmarshal; only the sibling RawPrevResult map[string]interface{} field (tag "prevResult,omitempty") actually receives the previous plugin's result. This is a pre-existing quirk of that library, not something introduced by this split, and existing code elsewhere in this repo already works around it by reading RawPrevResult directly (e.g. ops_check.go). inferFromPrevResult follows that same existing pattern. Left as-is rather than fixed here, since fixing it is unrelated to this split's scope. Taskfile.yaml, containers/galactic-cni/Dockerfile, and internal/installer/installer.go (SourceBGPBinary) gain galactic-bgp alongside galactic-cni/galactic-tap-cni/galactic-ipam, following the exact same pattern established for those two in prior steps. internal/cni/resource.go's resourceTracker drops all BGP/eBPF fields, leaving only vpc, vpcAttachment, vrfCreated, routesCreated — cleanup() no longer needs a Kubernetes client at all, since it never touches BGP CRDs. internal/cni/result.go's buildVethResult calls hostgw.Configure- HostGateway directly and returns only an error rather than threading an IPAMResult/MAC address back up for the caller to hand to a bgp helper that no longer lives in this package. Mirrored identically in internal/cnitap. Verification: task lint (0 issues), task build, task test:unit all green. task test:e2e not run in this step (matches prior two steps in this stack — requires sudo modprobe vrf plus a Kind cluster bring-up, deferred to the end of the full stack per the plan's verification approach).

Rollback context (blocking): rollbackCtx was created up front with a single 10s budget and reused for tracker.cleanup() after publishBGPState's own retryK8sOps could already burn ~30s across its retries. A slow API server could leave cleanup with an expired context, turning Delete's "NotFound" into "context deadline exceeded" (which client.IgnoreNotFound doesn't catch), leaking the just-created CRDs. rollbackCancel was also only called on the error branch, leaking a timer on every successful ADD. Now rollbackCtx is created fresh, with its own full budget, only inside the failure branch. CHECK coverage (blocking): checkEBPFEntry only read back vrf_table, so a corrupted/missing locator_table or function_table entry, or a nodeID that drifted out of range after ADD, still reported the attachment healthy. It now also verifies locator_table, function_table, and the nodeID range, matching what ADD treats as a hard error. e2e coverage (blocking): TestCNITapInterface's comment claimed eBPF registration happens inline in galactic-tap-cni's cmdAdd, which stopped being true with the plugin-chain split, and the test never exercised galactic-bgp at all -- silently losing coverage of BGP CRD creation and eBPF registration on the ADD path. It now chains /galactic-bgp ADD (fed the tap master's real result as prevResult) and CHECK after the tap master's own ADD, asserting the BGPVRFInstance/BGPAdvertisement CRDs exist and that CHECK -- which reads back all three eBPF tables -- passes. cniVersion constraint: type100.NewResult only accepts "1.0.0"/"1.1.0", and the master plugin echoes the conflist's own cniVersion straight into its printed Result, so an older value fails galactic-bgp's ADD for every attachment. Documented as an explicit, intentional requirement in prevresult.go and docs/cni/configuration.md, with a test locking in the rejection of older versions. DEL version/logging: cmdDel never parsed args.StdinData, always printed cniVersion "1.0.0" unconditionally (unlike internal/cni/ops_del.go, which only falls back to that on a parse failure), and never logged vpc/vpcAttachment. It now parses the conflist, logs the attachment, and uses pluginConf.CNIVersion with the same fallback-on-parse-failure pattern as the other DEL implementations in this chain. Interface-kind inference: veth/tap was inferred purely from len(Interfaces) (1 vs 2), enforced only in comments -- #306 chaining galactic-route into this same prevResult could add a host-side interface and silently misclassify tap as veth (or vice versa) instead of failing loudly, since both counts are valid switch cases. inferFromPrevResult now counts Sandbox-carrying interfaces instead: the actual property that distinguishes veth (guest end moved into the container netns) from tap (host-only), which survives an extra host-side interface without misclassifying. Smaller cleanup: - Removed the stale bgpv1alpha1.AddToScheme registration (with a circular justifying comment) from internal/cni/resource.go and internal/cnitap/resource.go -- neither package touches BGP CRDs anymore. - Added resource_test.go covering resourceTracker.cleanup's own wiring end to end (all three resource kinds, plus the rollback-collision race at that level) -- previously only the standalone unregisterEBPFDatapath was tested directly. - resourceTracker now embeds publishResult instead of cmdAdd copying its five tracking fields over one by one, so a future field added to one can't silently stop being tracked in the other. - Removed egressKindForInterfaceType's dead empty-string branch -- inferFromPrevResult always produces "veth" or "tap" now. Also added prevresult_test.go and ops_del_test.go, and introduced a package-level ebpfPinDir var (defaulting to attach.PinDir) so tests can redirect galactic-bgp's own eBPF registration/rollback/CHECK reads to a throwaway pin directory instead of the real production one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…leaved logs kubectl exec interleaves stdout and stderr, causing slog log lines from the CNI plugin chain to appear mixed with the JSON result. Use json.Decoder to parse only the first JSON value instead of json.Unmarshal on the full output. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

Rebased onto the updated fix/vrf-shared-per-vpc-1-core (which picked up my own rebase-and-reconcile of that branch after PR #307 moved out from under it) after that branch's history moved out from under this one -- same ripple as #306 -> #307 -> #311 -> #312. Conflicts resolved: this PR drops the eBPF vrf_table rollback path (registerEBPFDatapath no longer returns a block to track, unregisterEBPFDatapath is deleted entirely, publishResult loses ebpfRegistered/ebpfBlock/ebpfArgument) since the vrf_table entry is now shared per (VPC, node) same as the BGPVRFInstance CRD, so a failed ADD must never unregister it. The current tree had independently refactored resourceTracker to embed publishResult (rather than copying its fields one-by-one) between this PR's original base and now, so I kept that embedding -- it still holds exactly this PR's two surviving fields (advertisementCreated, vrfInstanceCreated) once the eBPF fields are gone -- and added this PR's own vrfInstanceCreated-conditioned-on-OperationResultCreated behavior and nodeName field on top of it, along with this PR's fuller cleanup() doc comment (it explains the shared-VRF reasoning more completely than what was already there). Also kept ebpfPinDir (the package-level var #305's own fix added for test injection) over the attach.PinDir literal this PR's diff predates, matching the same resolution #311 needed one level up. internal/cnibgp/resource_test.go needed a full rewrite rather than a per-hunk merge: it didn't exist yet at this PR's original base either (same gap #311 hit) -- it was added by #305's own fix commit -- so this PR's diff shows the file as "new," and the version already in the tree still tested the old unconditional-vrfInstanceCreated/ ebpfRegistered design this PR removes. Took this PR's four tests wholesale (they're purpose-built for the new design) and adjusted their resourceTracker literals for the embedded-publishResult shape (publishResult: publishResult{vrfInstanceCreated: true} instead of a bare vrfInstanceCreated: true field, which the embedding makes illegal in a keyed literal). Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nadpatch Rebased onto the updated refactor/cni-chain-4-installer-docs (which picked up PR #307's own review-feedback fix commit) after that branch's history moved out from under this one -- same ripple as Conflicts resolved: internal/cnibgp/bgp.go, ops_check.go, and resource.go each had an import-block conflict from this PR's internal/cni/crdnames -> internal/crdnames promotion landing on lines the current tree had already changed independently (PR #305's own fix commit moved cnibgp's eBPF pin-dir handling behind a package- level ebpfPinDir var in cnibgp.go, so bgp.go/ops_check.go/resource.go no longer import internal/plumbing/ebpf/attach directly the way this PR's diff -- authored before that -- expected). Resolved by applying just the crdnames rename to each file and leaving the attach import out, matching how the current tree already gets ebpfPinDir. Also fixed one file this PR's diff never touched: internal/cnibgp/ resource_test.go didn't exist yet at this PR's original base -- it was added later by #305's own fix commit -- so its own "go.datum.net/galactic/internal/cni/crdnames" import needed the same promotion this PR already applied everywhere else, or the package wouldn't build. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the three promoted packages (internal/crdnames, internal/hostconf, internal/nadpatch). tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nimaster Rebased onto the updated fix/cni-review-followups-doc-placement (which picked up my own rebase-and-reconcile of that branch after PR from under this one -- same ripple as #306 -> #307 -> #315 -> #316. Conflicts resolved: internal/cni/resource.go and internal/cnitap/ resource.go each had two independent changes touching the same resourceTracker/cleanup() region -- this PR's own extraction of the shared k8s-client-construction (newK8sClient/cniScheme) and interface+ VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an IPAM-release rollback step (this PR's diff predates that feature entirely, same gap #311/#312/#315 each hit one level up). Kept both: call cnimaster.CleanupAttachment for the interface+VRF half (this PR's whole point), and kept the IPAM release step ahead of it, unchanged. internal/cnitap/ops_check.go had one similar import-only conflict (this PR drops the netlink/rest/ctrl/vrf imports cnimaster. CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) -- its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this- PR gap) sat entirely outside the conflicted hunk and needed no resolution beyond keeping the "github.com/containernetworking/plugins/ pkg/ipam" import alive. Verification: task lint, task build (all 8 binaries), task test:unit all pass on the rebased tree, including the new internal/cnimaster package. tests/e2e not run in this sandbox (no Kind cluster / root), same caveat as every PR in this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

privateip requested a review from a team as a code owner August 8, 2026 04:22

privateip requested review from ronggur and removed request for a team August 8, 2026 04:22

privateip mentioned this pull request Aug 8, 2026

refactor(cni): extract galactic-route as its own CNI chain plugin #306

Merged

privateip force-pushed the refactor/cni-chain-2-galactic-bgp branch from 40ab587 to d1a6f68 Compare August 8, 2026 12:02

This was referenced Aug 8, 2026

refactor(cni): split galactic-cni into veth/tap master plugins #303

Merged

refactor(cni): extract galactic-ipam as a real delegated CNI IPAM plugin #304

Merged

docs+fixes(cni): finish the CNI plugin-chain split — docs, containerlab, dead code #307

Merged

privateip force-pushed the refactor/cni-chain-2-galactic-bgp branch from d1a6f68 to f67ee3f Compare August 8, 2026 12:15

privateip mentioned this pull request Aug 8, 2026

Galactic CNI: publish per-pod SRv6 SID + tenant identifier via EndpointSlice for VPC HTTP ingress backends datum-cloud/enhancements#854

Closed

8 tasks

privateip force-pushed the refactor/cni-chain-2-galactic-bgp branch 2 times, most recently from fd81bfd to e33092c Compare August 8, 2026 12:37

privateip requested review from 0xmc, scotwells and slindseysr August 8, 2026 12:44

privateip force-pushed the refactor/cni-chain-2-galactic-bgp branch from e33092c to 956b547 Compare August 8, 2026 19:03

privateip force-pushed the refactor/cni-chain-2-galactic-bgp branch from 7c6fad1 to 7cf773a Compare August 9, 2026 20:01

ecv mentioned this pull request Aug 10, 2026

An incomplete plugin chain still reports a successful attach #331

Open

ecv previously approved these changes Aug 10, 2026

View reviewed changes

privateip dismissed ecv’s stale review via bb3e0ea August 10, 2026 17:51

privateip force-pushed the refactor/cni-chain-2-galactic-bgp branch from ae755f3 to bb3e0ea Compare August 10, 2026 17:51

privateip requested review from 0xmc and ecv August 10, 2026 18:19

privateip force-pushed the refactor/cni-chain-2-galactic-bgp branch from bb3e0ea to d72a33e Compare August 10, 2026 18:27

Base automatically changed from refactor/cni-chain-1-galactic-ipam to main August 10, 2026 18:27

privateip and others added 3 commits August 10, 2026 14:27

privateip force-pushed the refactor/cni-chain-2-galactic-bgp branch from d72a33e to fb74dc2 Compare August 10, 2026 18:28

ecv approved these changes Aug 10, 2026

View reviewed changes

privateip merged commit a55169d into main Aug 10, 2026
10 checks passed

privateip deleted the refactor/cni-chain-2-galactic-bgp branch August 10, 2026 18:44

Navigation Menu

Uh oh!

refactor(cni): extract galactic-bgp as its own CNI chain plugin - #305

refactor(cni): extract galactic-bgp as its own CNI chain plugin#305
privateip merged 3 commits into
mainfrom
refactor/cni-chain-2-galactic-bgp

privateip commented Aug 8, 2026 •

edited

Loading

Uh oh!

mattdjenkinson commented Aug 8, 2026

Uh oh!

privateip commented Aug 9, 2026

Uh oh!

ecv left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

privateip commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What moved

Rollback scoping

hostgw extraction

PrevResult vs RawPrevResult

Verification

Uh oh!

mattdjenkinson commented Aug 8, 2026

Uh oh!

privateip commented Aug 9, 2026

Uh oh!

ecv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

privateip commented Aug 8, 2026 •

edited

Loading